Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the diagnostics for unused generic params in ty aliases & permit bivariant but constrained params in LTAs #120556

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Feb 1, 2024

  • Don't emit two errors (namely E0091 and E0392) for unused type parameters on lazy type aliases
  • Fix the diagnostic help message of E0392 for lazy type aliases: Don't talk about the “fields” of lazy type aliases (use the term “body” instead) and don't suggest PhantomData for them, it doesn't make much sense
  • Consolidate the diagnostics for E0091 (unused type parameters in type aliases) and E0392 (unused generic parameters due to bivariance) and make it translatable
    • Still keep the error codes distinct (for now)
    • Naturally leads to better diagnostics for E0091

Edit(2024-07-26): This PR starts allowing bivariant but constrained params in lazy type aliases just like in ADTs. E.g., type A<T, F> = F where F: Fn() -> T; (similar to struct S<T, F>(F) where F: Fn() -> T;); T is bivariant but constrained. E0091 for eager type aliases doesn't account for such a thing.

r? @oli-obk (to ballast your review load :P) or compiler

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 1, 2024
)
} else {
format!("consider removing `{param_name}` or referring to it in a field")
let param_name = param.name.ident();
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no longer extracting the Symbol from the Ident via param.name.ident().name since the Display impl for Ident accounts for escaping via r#. This way, we properly display r#if in type T<r#if> = (); as r#if instead of if.

@@ -10,14 +10,14 @@ error: expected identifier, found reserved identifier `_`
LL | fn bad_infer_fn<_>() {}
| ^ expected identifier, found reserved identifier

error[E0392]: parameter `_` is never used
error[E0392]: type parameter `_` is never used
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add extra code for skipping this error entirely by delaying a bug on _ when checking the “usedness”. Not sure if it'd be worth it though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be very hard to track the expected identifier, found reserved identifier error all the way into the type system. We'd probably have to add a GenericParamDefKind::Error to do that. May be worth exploring, but not in this PR

Comment on lines +1307 to +1311
// FIXME: This assumes that elaborated `Sized` bounds come first (which does hold at the
// time of writing). This is a bit fragile since we later use the span to detect elaborated
// `Sized` bounds. If they came last for example, this would break `Trait + /*elab*/Sized`
// since it would overwrite the span of the user-written bound. This could be fixed by
// folding the spans with `Span::to` which requires a bit of effort I think.
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't want to deal with this right now but I can try to do so if you'd like me to. I think it'll be a bit nasty to implement though. Consider a hypothetical set of predicates like [<A as TraitA>, <B as TraitB>, /*elab*/<A as Sized>] (note: we need to merge the spans of the 0th and the 2nd predicate).

Maybe it's actually not that ugly to implement, I just haven't spent any time to think about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it rely on that? the spans differ, so both elaborated and user-written would end up in the hash set

Copy link
Contributor

@oli-obk oli-obk Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 it's a hashmap and this collect relies on the hash map impl overwriting with follow up items of the same key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine to rely on this, there are tests after all

@fmease fmease force-pushed the improve-unused-generic-param-diags branch from 7a4f93d to 02320b5 Compare February 1, 2024 15:18
@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 1, 2024

📌 Commit 02320b5 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 1, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2024
…m-diags, r=oli-obk

Improve the diagnostics for unused generic parameters

* Don't emit two errors (namely E0091 *and* E0392) for unused type parameters on *lazy* type aliases
* Fix the diagnostic help message of E0392 for *lazy* type aliases: Don't talk about the “fields” of lazy type aliases (use the term “body” instead) and don't suggest `PhantomData` for them, it doesn't make much sense
* Consolidate the diagnostics for E0091 (unused type parameters in type aliases) and E0392 (unused generic parameters due to bivariance) and make it translatable
  * Still keep the error codes distinct (for now)
  * Naturally leads to better diagnostics for E0091

r? `@oli-obk` (to ballast your review load :P) or compiler
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2024
…m-diags, r=oli-obk

Improve the diagnostics for unused generic parameters

* Don't emit two errors (namely E0091 *and* E0392) for unused type parameters on *lazy* type aliases
* Fix the diagnostic help message of E0392 for *lazy* type aliases: Don't talk about the “fields” of lazy type aliases (use the term “body” instead) and don't suggest `PhantomData` for them, it doesn't make much sense
* Consolidate the diagnostics for E0091 (unused type parameters in type aliases) and E0392 (unused generic parameters due to bivariance) and make it translatable
  * Still keep the error codes distinct (for now)
  * Naturally leads to better diagnostics for E0091

r? ``@oli-obk`` (to ballast your review load :P) or compiler
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120520 (Some cleanups around diagnostic levels.)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120473 (Only suggest removal of `as_*` and `to_` conversion methods on E0308)
 - rust-lang#120540 (add test for try-block-in-match-arm)
 - rust-lang#120547 (`#![feature(inline_const_pat)]` is no longer incomplete)
 - rust-lang#120552 (Correctly check `never_type` feature gating)
 - rust-lang#120555 (put pnkfelix (me) back on the review queue.)
 - rust-lang#120556 (Improve the diagnostics for unused generic parameters)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7fa99bf into rust-lang:master Feb 5, 2024
11 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
Rollup merge of rust-lang#120556 - fmease:improve-unused-generic-param-diags, r=oli-obk

Improve the diagnostics for unused generic parameters

* Don't emit two errors (namely E0091 *and* E0392) for unused type parameters on *lazy* type aliases
* Fix the diagnostic help message of E0392 for *lazy* type aliases: Don't talk about the “fields” of lazy type aliases (use the term “body” instead) and don't suggest `PhantomData` for them, it doesn't make much sense
* Consolidate the diagnostics for E0091 (unused type parameters in type aliases) and E0392 (unused generic parameters due to bivariance) and make it translatable
  * Still keep the error codes distinct (for now)
  * Naturally leads to better diagnostics for E0091

r? ```@oli-obk``` (to ballast your review load :P) or compiler
@rustbot rustbot added this to the 1.78.0 milestone Feb 5, 2024
@fmease fmease deleted the improve-unused-generic-param-diags branch February 5, 2024 01:11
@fmease fmease changed the title Improve the diagnostics for unused generic parameters Improve the diagnostics for unused generic parameters in type aliases & permit bivariant but constrained params in LTAs Jul 26, 2024
@fmease fmease added the F-lazy_type_alias `#![feature(lazy_type_alias)]` label Jul 26, 2024
@fmease fmease changed the title Improve the diagnostics for unused generic parameters in type aliases & permit bivariant but constrained params in LTAs Improve the diagnostics for unused generic params in ty aliases & permit bivariant but constrained params in LTAs Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-lazy_type_alias `#![feature(lazy_type_alias)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants